Skip to content

Update query-metadata-style-guide.md#19020

Merged
jonjanego merged 6 commits intomainfrom
jonjanego-patch-1
Apr 1, 2025
Merged

Update query-metadata-style-guide.md#19020
jonjanego merged 6 commits intomainfrom
jonjanego-patch-1

Conversation

@jonjanego
Copy link
Copy Markdown
Member

initial commit of changes starting to add quality tagging standards

initial commit of changes starting to add quality tagging standards
@LWSimpkins
Copy link
Copy Markdown
Contributor

Would it be worth adding there is a limit of 10 tags (including the automatically added CWE tags)?

Otherwise, there's a warning message when the sarif is parsed/alerts for that query are displayed in the UI. (At least when calling the API; not sure about sarif uploads from the GitHub action).

Comment thread docs/query-metadata-style-guide.md
minor tag changes to align with existing tags
@jonjanego
Copy link
Copy Markdown
Member Author

Would it be worth adding there is a limit of 10 tags (including the automatically added CWE tags)?

Otherwise, there's a warning message when the sarif is parsed/alerts for that query are displayed in the UI. (At least when calling the API; not sure about sarif uploads from the GitHub action).

I don't see any problem with adding that, i can update

@jonjanego jonjanego marked this pull request as ready for review April 1, 2025 19:59
@jonjanego jonjanego requested a review from a team as a code owner April 1, 2025 19:59
adityasharad
adityasharad previously approved these changes Apr 1, 2025
Copy link
Copy Markdown
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good in general, minor suggestions and we can keep improving over time.

Comment thread docs/query-metadata-style-guide.md Outdated

Maintainers are expected to add a `@security-severity` tag to security relevant queries that will be run on Code Scanning. There is a documented internal process for generating these `@security-severity` values.

TODO: should we have a severity value for quality queries?
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a non-security problem.severity field, which can be error/warning/recommendation/none.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A while ago we intended severity to be the new spelling of problem.severity (and @kind alert to replace @kind problem). This document is even older than that and we never got around to switching existing queries to the new spelling but should we change this to those or do we want to go back on those changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alexet - ah... i did not know that history. I'm open to whatever the team thinks is best. @yo-h and @adityasharad , do you have opinions of what to do here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also did not know that history, so I have been adding queries with @problem.severity and @kind {path-}problem for the past 2+ years. 😅 It looks like those changes were made in 2021 (https://github.com/github/semmle-code/pull/39133, https://github.com/github/semmle-code/pull/39132), but when I joined in 2022, I was directed to this metadata style guide and metadata-for-codeql-queries which both still only listed problem.severity and {path-}problem as options.

should we change this to those or do we want to go back on those changes

Searching in the codeql repo shows only a handful of queries using @severity (mostly model-generator related queries) and no queries using @kind path-alert or @kind alert. Since it's been four years since these changes were made, and they haven't been used much, maybe it makes sense to go back on them? Or at least not prioritize them now?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the CLI recognises those variants, but we never changed the guidance and existing practices :) I am fine keeping the aliases around so we can change over later to the newer names, but no need to rewrite any existing code.

Comment thread docs/query-metadata-style-guide.md
@jonjanego jonjanego requested a review from adityasharad April 1, 2025 20:43
@jonjanego jonjanego removed the request for review from yo-h April 1, 2025 20:50
@jonjanego jonjanego merged commit fa02f82 into main Apr 1, 2025
6 checks passed
@jonjanego jonjanego deleted the jonjanego-patch-1 branch April 1, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants